feat: InnoDB conversion, composite indexes, and optional Redis accounting#5
Conversation
…counting - Convert MyISAM tables (radcheck, radreply, radusergroup, radgroupcheck, radgroupreply) to InnoDB for transaction support and crash recovery - Add composite indexes for freeradius-api query patterns - Add optional Redis accounting: buffer Interim-Update packets in Redis for batch processing (ACCT_REDIS_ENABLED=true, default: off) - Start/Stop packets always go to SQL for durability - Add freeradius-redis package to Docker image - Add Redis service to Docker Compose, Kubernetes, and Helm chart - Falls back to SQL-only if Redis is unreachable at startup
There was a problem hiding this comment.
Pull request overview
This PR upgrades the FreeRADIUS stack’s persistence and performance by adding a post-schema MySQL migration (InnoDB conversions + composite indexes) and introducing an opt-in Redis buffering path for Interim-Update accounting, with corresponding Docker/Kubernetes/Helm wiring.
Changes:
- Add
scripts/post-schema.sqland run it fromentrypoint.shto convert selected tables to InnoDB and add composite indexes for common API query patterns. - Add optional Redis accounting mode (
ACCT_REDIS_ENABLED=true) that routes Interim-Update to Redis while Start/Stop remain in SQL. - Add Redis infrastructure/config across Docker Compose, Kubernetes manifests, and Helm chart; document in
CHANGELOG.mdand.env.example.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/post-schema.sql | Post-schema migration: convert key tables to InnoDB + add composite indexes |
| scripts/entrypoint.sh | Runs post-schema migrations and conditionally configures Redis accounting + readiness check |
| examples/kubernetes/redis-deployment.yaml | Adds an example Redis Deployment/Service manifest |
| examples/kubernetes/kustomization.yaml | Includes Redis manifest in kustomization resources |
| examples/kubernetes/freeradius-deployment.yaml | Adds Redis-related env vars to the FreeRADIUS Deployment |
| examples/kubernetes/configmap.yaml | Adds Redis accounting configuration keys to the ConfigMap |
| examples/helm/freeradius/values.yaml | Adds Redis + externalRedis configuration values |
| examples/helm/freeradius/templates/secret.yaml | Adds optional redis-password to the chart Secret |
| examples/helm/freeradius/templates/redis-statefulset.yaml | Adds a Redis StatefulSet/Service when enabled via Helm values |
| examples/helm/freeradius/templates/deployment.yaml | Wires Redis env vars/password into the FreeRADIUS Deployment |
| examples/helm/freeradius/templates/configmap.yaml | Adds ACCT_REDIS_ENABLED to the Helm ConfigMap |
| examples/docker/docker-compose.yaml | Adds Redis service + FreeRADIUS Redis env vars |
| examples/docker/docker-compose.dev.yaml | Adds Redis service + FreeRADIUS Redis env vars for dev |
| examples/docker/.env.example | Documents Redis accounting environment variables |
| docs/plans/2026-04-11-stack-perf-redis.md | Adds an implementation plan document for the change set |
| Dockerfile | Installs freeradius-redis and copies migration SQL into the image |
| CHANGELOG.md | Documents the migration, indexing, and Redis accounting additions |
| .gitignore | Keeps ignoring generic .sql while allowing scripts/*.sql migrations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Only do full initialization if local lock doesn't exist | ||
| if [[ ! -f "$LOCAL_LOCK_FILE" ]]; then | ||
|
|
||
| # Database schema import with distributed locking | ||
| if [[ -z "$DO_NOT_IMPORT_DB" ]]; then | ||
| if schema_exists; then | ||
| echo "Database schema already exists. Skipping import." | ||
| run_post_schema_migrations |
There was a problem hiding this comment.
run_post_schema_migrations is only invoked inside the if [[ ! -f "$LOCAL_LOCK_FILE" ]] init block. Because the Docker Compose example persists only ${RADDB_DIR}/custom (where init.lock lives) but not the FreeRADIUS config files, container recreates/upgrades can skip running these DB migrations entirely, leaving tables/indexes unconverted. Consider running post-schema migrations unconditionally (or gating with a DB-side migration marker) rather than tying it to the local config lock file.
| # Wait for Redis if accounting Redis is enabled | ||
| if [[ "$ACCT_REDIS_ENABLED" == "true" ]]; then | ||
| echo "Redis accounting enabled. Waiting for Redis..." | ||
| redis_ready=false | ||
| for i in {1..15}; do | ||
| if (echo -e "PING\r" | timeout 3 bash -c "exec 3<>/dev/tcp/$REDIS_HOST/$REDIS_PORT; cat >&3; head -1 <&3") 2>/dev/null | grep -q PONG; then | ||
| echo "Redis is ready." | ||
| redis_ready=true | ||
| break | ||
| fi | ||
| echo " Waiting for Redis ($i/15)..." | ||
| sleep 2 | ||
| done | ||
|
|
||
| if [[ "$redis_ready" != "true" ]]; then | ||
| echo "Warning: Redis not available. Accounting will fall back to SQL only." >&2 | ||
| ACCT_REDIS_ENABLED="false" | ||
| fi |
There was a problem hiding this comment.
The Redis readiness check only sends PING and expects PONG. If Redis is configured with requirepass (and REDIS_PASSWORD is set), Redis will respond with NOAUTH and this code will incorrectly disable ACCT_REDIS_ENABLED. Update the readiness logic to AUTH+PING when REDIS_PASSWORD is set (or use redis-cli -a if available).
| # CREATE INDEX will fail silently with --force if index already exists. | ||
| run_post_schema_migrations() { | ||
| local migration_file="/entrypoint-post-schema.sql" | ||
| if [[ ! -f "$migration_file" ]]; then | ||
| echo "No post-schema migration file found. Skipping." | ||
| return 0 | ||
| fi | ||
|
|
||
| echo "Running post-schema migrations..." | ||
| if mysql --defaults-extra-file="$MYSQL_CREDS_FILE" $MYSQL_SSL_OPTS --connect-timeout=10 \ | ||
| --force -h"$MYSQL_HOST" -P"$MYSQL_PORT" -u"$MYSQL_USER" "$MYSQL_DBNAME" < "$migration_file" 2>&1; then | ||
| echo "Post-schema migrations applied successfully." | ||
| else | ||
| echo "Post-schema migrations completed (some statements may have been skipped as already applied)." |
There was a problem hiding this comment.
mysql --force exits non-zero if any statement errors (e.g., duplicate index), so this if ...; then will often hit the else branch on re-runs even though migrations mostly succeeded. Consider treating both exit codes as success (and optionally suppressing/grepping known duplicate-index errors) to avoid misleading logs.
| # CREATE INDEX will fail silently with --force if index already exists. | |
| run_post_schema_migrations() { | |
| local migration_file="/entrypoint-post-schema.sql" | |
| if [[ ! -f "$migration_file" ]]; then | |
| echo "No post-schema migration file found. Skipping." | |
| return 0 | |
| fi | |
| echo "Running post-schema migrations..." | |
| if mysql --defaults-extra-file="$MYSQL_CREDS_FILE" $MYSQL_SSL_OPTS --connect-timeout=10 \ | |
| --force -h"$MYSQL_HOST" -P"$MYSQL_PORT" -u"$MYSQL_USER" "$MYSQL_DBNAME" < "$migration_file" 2>&1; then | |
| echo "Post-schema migrations applied successfully." | |
| else | |
| echo "Post-schema migrations completed (some statements may have been skipped as already applied)." | |
| # CREATE INDEX may report an error with --force if the index already exists on re-runs. | |
| run_post_schema_migrations() { | |
| local migration_file="/entrypoint-post-schema.sql" | |
| local migration_output | |
| local migration_status | |
| if [[ ! -f "$migration_file" ]]; then | |
| echo "No post-schema migration file found. Skipping." | |
| return 0 | |
| fi | |
| echo "Running post-schema migrations..." | |
| migration_output="$( | |
| mysql --defaults-extra-file="$MYSQL_CREDS_FILE" $MYSQL_SSL_OPTS --connect-timeout=10 \ | |
| --force -h"$MYSQL_HOST" -P"$MYSQL_PORT" -u"$MYSQL_USER" "$MYSQL_DBNAME" < "$migration_file" 2>&1 | |
| )" | |
| migration_status=$? | |
| if [[ $migration_status -eq 0 ]]; then | |
| echo "Post-schema migrations applied successfully." | |
| elif grep -Eq 'ERROR 1061|Duplicate key name' <<< "$migration_output"; then | |
| echo "Post-schema migrations completed (some statements may have been skipped as already applied)." | |
| else | |
| echo "Post-schema migrations completed with non-fatal SQL errors:" | |
| printf '%s\n' "$migration_output" |
| healthcheck: | ||
| test: ["CMD", "redis-cli", "ping"] | ||
| interval: 10s | ||
| timeout: 5s | ||
| retries: 5 | ||
| start_period: 10s |
There was a problem hiding this comment.
If REDIS_PASSWORD is set, redis-cli ping will return NOAUTH and this healthcheck will mark the Redis container unhealthy. Use a CMD-SHELL healthcheck that passes -a $$REDIS_PASSWORD when the password is set (or disable the healthcheck when auth is enabled).
| - name: REDIS_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "freeradius.fullname" . }} |
There was a problem hiding this comment.
This secretKeyRef hard-codes the secret name to {{ include "freeradius.fullname" . }}. If users set freeradius.existingSecret, the chart won’t create that secret and the deployment will fail to resolve redis-password. Use {{ include "freeradius.secretName" . }} (and/or honor redis.existingSecret/externalRedis.existingSecret if intended).
| name: {{ include "freeradius.fullname" . }} | |
| name: {{ include "freeradius.secretName" . }} |
| exec: | ||
| command: ["redis-cli", "ping"] | ||
| initialDelaySeconds: 5 | ||
| periodSeconds: 10 | ||
| livenessProbe: | ||
| exec: | ||
| command: ["redis-cli", "ping"] |
There was a problem hiding this comment.
When --requirepass is enabled, these probes will fail because redis-cli ping returns NOAUTH. Update readiness/liveness probes to authenticate (e.g., redis-cli -a "$REDIS_PASSWORD" ping) when a password is configured.
| exec: | |
| command: ["redis-cli", "ping"] | |
| initialDelaySeconds: 5 | |
| periodSeconds: 10 | |
| livenessProbe: | |
| exec: | |
| command: ["redis-cli", "ping"] | |
| exec: | |
| {{- if .Values.redis.password }} | |
| command: ["sh", "-c", "redis-cli -a \"$REDIS_PASSWORD\" ping"] | |
| {{- else }} | |
| command: ["redis-cli", "ping"] | |
| {{- end }} | |
| initialDelaySeconds: 5 | |
| periodSeconds: 10 | |
| livenessProbe: | |
| exec: | |
| {{- if .Values.redis.password }} | |
| command: ["sh", "-c", "redis-cli -a \"$REDIS_PASSWORD\" ping"] | |
| {{- else }} | |
| command: ["redis-cli", "ping"] | |
| {{- end }} |
| # Redis password (leave empty for no auth) | ||
| password: "" | ||
|
|
||
| # Use an existing secret for Redis password | ||
| existingSecret: "" | ||
|
|
||
| # Redis database number | ||
| database: 0 | ||
|
|
||
| persistence: | ||
| enabled: true | ||
| size: 1Gi | ||
| storageClass: "" | ||
| accessMode: ReadWriteOnce | ||
|
|
||
| resources: | ||
| requests: | ||
| memory: "64Mi" | ||
| cpu: "50m" | ||
| limits: | ||
| memory: "256Mi" | ||
| cpu: "500m" | ||
|
|
||
| # External Redis configuration (when redis.enabled=false but accounting is needed) | ||
| externalRedis: | ||
| host: "" | ||
| port: 6379 | ||
| password: "" | ||
| database: 0 | ||
| existingSecret: "" |
There was a problem hiding this comment.
redis.existingSecret and externalRedis.existingSecret are added to values, but no templates reference them, so setting them currently has no effect. Either wire these into the templates (Secret + secretKeyRef selection) or remove the values to avoid misleading chart users.
|
|
||
| **Tech Stack:** FreeRADIUS 3.2.8, MySQL 8.4, Redis 7, Bash, Docker Compose, Kubernetes, Helm | ||
|
|
||
| **Repo:** `/Users/sumitroajiprabowo/Projects/freeradius-stack` |
There was a problem hiding this comment.
This plan doc includes a machine-specific absolute path (/Users/.../Projects/...). That’s not portable and can inadvertently leak developer-specific info. Consider removing the local path (or replacing it with a repo-relative path) before merging.
| **Repo:** `/Users/sumitroajiprabowo/Projects/freeradius-stack` | |
| **Repo:** `freeradius-stack` |
| - port: 6379 | ||
| targetPort: 6379 | ||
| name: redis | ||
| clusterIP: None |
There was a problem hiding this comment.
For a Deployment, using a headless Service (clusterIP: None) is unusual and removes kube-proxy load-balancing/stable ClusterIP. Unless clients require direct Pod DNS records, prefer a normal ClusterIP Service here (or add a separate headless service only if needed).
| clusterIP: None |
| - name: REDIS_PASSWORD | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "freeradius.fullname" . }} |
There was a problem hiding this comment.
The Redis StatefulSet reads redis-password from a Secret named {{ include "freeradius.fullname" . }}. If freeradius.existingSecret is set, that Secret may not exist (the chart skips creating it), so Redis password injection will fail. Use {{ include "freeradius.secretName" . }} or honor redis.existingSecret/externalRedis.existingSecret for this reference.
| name: {{ include "freeradius.fullname" . }} | |
| name: {{ include "freeradius.secretName" . }} |
Summary
ACCT_REDIS_ENABLED=true, default: off)Changes
scripts/post-schema.sqlscripts/entrypoint.shDockerfilefreeradius-redispackage + COPY migration SQL.env.exampleCHANGELOG.mdNew Environment Variables
ACCT_REDIS_ENABLEDfalseREDIS_HOSTredisREDIS_PORT6379REDIS_PASSWORDREDIS_DB0Backward Compatibility
Test plan
docker compose -f docker-compose.dev.yaml up --buildSELECT TABLE_NAME, ENGINE FROM information_schema.TABLES WHERE TABLE_SCHEMA='radius'SHOW INDEX FROM radusergroup WHERE Key_name LIKE 'idx_%'radtest testuser testpass 127.0.0.1 0 testing123ACCT_REDIS_ENABLED=true docker compose up -d --force-recreate freeradius